Alter text for view-only users is ReportError. #6292
Conversation
|
Size Change: +836 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
@sashadoes I have requested one small change regarding the CTA display. Thank you for your work on this!
There was a problem hiding this comment.
@sashadoes It looks like updating the showRequestAccessURL variable was missed out. It also needs to be falsey for a view-only user.
Otherwise, there is a chance it could show the Request access CTA for a view-only user, which this issue is trying to prevent. See screenshot:
Back to you for another update. Thanks!
There was a problem hiding this comment.
@sashadoes This looks good so far.
I apologise for not noticing this earlier, it looks like adding a test case for this change was mentioned in the IB, but it was missed in the implementation. Could you add a test case covering the change in assets/js/components/ReportError.test.js?
Thanks for your patience!
|
@nfmohit makes sense , thanks :) |
There was a problem hiding this comment.
Thank you, @sashadoes. I've left a few comments on the test case.
| { | ||
| registry, | ||
| }, | ||
| { |
There was a problem hiding this comment.
The render function only accepts two parameters. The viewContext property needs to be adjacent to the registry property in the second parameter object.
| const { queryByText } = render( | ||
| <ReportError | ||
| moduleSlug={ moduleName } | ||
| error={ { |
There was a problem hiding this comment.
While this test does pass, but it doesn't serve our purpose of coverage. This <ReportError /> component doesn't show the Retry button anyway, in any view context. For it to show, the selector should be getReport.
There was a problem hiding this comment.
In order to avoid the back-and-forth, I've made the changes myself. Thank you, @sashadoes!
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist